Update golangci-lint, add more linters #1267

aLekSer · 2020-01-10T12:46:12Z

Fix minor issues, but plenty of them. Add new linters.
Add pointers when it is necessary for the sake of performance (avoid coping structures over 512 B in size).
Most fixes come from gocritic linter.
https://go-critic.github.io/overview.html

agones-bot · 2020-01-10T13:02:59Z

Build Failed 😱

Build Id: fe238fe9-c5ea-4282-8699-3d0e44f90102

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-01-10T13:35:55Z

Build Succeeded 👏

Build Id: 363b095d-97cc-4fe5-8cd5-12b42f6faffd

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-6f37730
image: gcr.io/agones-images/agones-sdk:1.3.0-6f37730
image: gcr.io/agones-images/agones-ping:1.3.0-6f37730
Linux C++ SDK (build): agonessdk-1.3.0-6f37730-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-6f37730.zip

A preview of the website (the last 30 builds are retained):

https://6f37730-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1267/head:pr_1267 && git checkout pr_1267
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-6f37730

roberthbailey

The only change I'm not sure about is adding in named return values (which I generally try to avoid). This happened in a couple of files (but not in a bunch of other places) so I can't tell what heuristic is being used to suggest the changes.

roberthbailey · 2020-01-10T18:34:58Z

cmd/allocator/main.go

@@ -212,21 +212,21 @@ func newServiceHandler(kubeClient kubernetes.Interface, agonesClient versioned.I
 }

 // Set up our client which we will use to call the API
-func getClients() (*kubernetes.Clientset, *versioned.Clientset, error) {
+func getClients() (kubeClient *kubernetes.Clientset, agonesClient *versioned.Clientset, err error) {


I don't understand the suggested changes to this function; the old way looks more correct to me stylistically. Do you know which check triggered this?

Yeah, I'm not a fan of named return variables either.

I think the idea behind this check was to make function signature more readable from the first line, especially when it returns same 3 types for instance, which can be misleading without names.
But as we have 3 votes to not use them I will disable unnamedResult and update the PR.

roberthbailey · 2020-01-10T18:46:41Z

.golangci.yml

+      - performance
+      - opinionated
+      - diagnostic
+    disabled-checks:


Did these checks produce too many diffs or did they produce diffs that you didn't agree with?

Most of the changes was from test helper functions, that's why I have added:

rangeValCopy: sizeThreshold: 512

to filter them a bit, and to split the changes into 2 PRs also, but I can include these changes here.
For instance, I don't like paramTypeCombine, especially in this case:

pkg/gameservers/controller.go:91:1: paramTypeCombine: func( wh *webhooks.WebHook, health healthcheck.Handler, minPort, maxPort int32, sidecarImage string, alwaysPullSidecarImage bool, sidecarCPURequest resource.Quantity, sidecarCPULimit resource.Quantity, sdkServiceAccount string, kubeClient kubernetes.Interface, kubeInformerFactory informers.SharedInformerFactory, extClient extclientset.Interface, agonesClient versioned.Interface, agonesInformerFactory externalversions.SharedInformerFactory) *Controller could be replaced with func(wh *webhooks.WebHook, health healthcheck.Handler, minPort, maxPort int32, sidecarImage string, alwaysPullSidecarImage bool, sidecarCPURequest, sidecarCPULimit resource.Quantity, sdkServiceAccount string, kubeClient kubernetes.Interface, kubeInformerFactory informers.SharedInformerFactory, extClient extclientset.Interface, agonesClient versioned.Interface, agonesInformerFactory externalversions.SharedInformerFactory) *Controller (gocritic)

Seems it just combined sidecarCPURequest, sidecarCPULimit resource.Quantity,, but I can add this check also if it is needed.

sloppyReassign example:

pkg/gameservers/controller.go:389:5: sloppyReassign: re-assignment to `err` can be replaced with `err := c.syncGameServerShutdownState(gs)` (gocritic) if err = c.syncGameServerShutdownState(gs); err != nil {

For the paramTypeCombine check, the common suggestion I've heard is that when parameter lists get that long you should create a struct and pass the struct instead of a list of individual parameters. The reason being that it adds better type checking, and when the parameter list gets long it becomes easy to pass things in the wrong order, especially if parameters have the same type. This check seems to make that worse, so I agree that we shouldn't enable it.

The change in sloppyReassign looks correct to me.

agones-bot · 2020-01-11T10:00:46Z

Build Failed 😱

Build Id: 064f2294-b852-4371-b7b3-1c7b3f0bf93f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-01-11T10:00:55Z

Build Failed 😱

Build Id: fe688c0b-22cf-4af7-8fc9-9d475f0075c7

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-01-11T10:21:17Z

Build Failed 😱

Build Id: b999bdd2-b381-450e-b99b-42bb60dbfa86

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-01-11T10:32:09Z

Build Succeeded 👏

Build Id: cad4e15f-ac27-49b8-b5de-464334492120

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-cb01d82
image: gcr.io/agones-images/agones-sdk:1.3.0-cb01d82
image: gcr.io/agones-images/agones-ping:1.3.0-cb01d82
Linux C++ SDK (build): agonessdk-1.3.0-cb01d82-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-cb01d82.zip

A preview of the website (the last 30 builds are retained):

https://cb01d82-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1267/head:pr_1267 && git checkout pr_1267
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-cb01d82

aLekSer · 2020-01-11T11:54:15Z

Some error on TestAllocatorCrossNamespace E2E:

Step #21: allocator_test.go:210: failed to unmarshall response body:
 Post https://34.82.102.232/v1alpha1/gameserverallocation:
 remote error: tls: bad certificate

https://gist.github.com/aLekSer/87c8f03ce7af07063866f994f30bad2f

agones-bot · 2020-01-11T12:39:33Z

Build Succeeded 👏

Build Id: eff681ad-c1ee-4ec4-964e-d170dc78acaf

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-f65c6e8
image: gcr.io/agones-images/agones-sdk:1.3.0-f65c6e8
image: gcr.io/agones-images/agones-ping:1.3.0-f65c6e8
Linux C++ SDK (build): agonessdk-1.3.0-f65c6e8-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-f65c6e8.zip

A preview of the website (the last 30 builds are retained):

https://f65c6e8-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1267/head:pr_1267 && git checkout pr_1267
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-f65c6e8

cmd/allocator/main.go

test/e2e/allocator_test.go

agones-bot · 2020-01-13T08:42:41Z

Build Succeeded 👏

Build Id: a5a6372f-7234-4bcd-8e8b-64a489d04201

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-1a3c089
image: gcr.io/agones-images/agones-sdk:1.3.0-1a3c089
image: gcr.io/agones-images/agones-ping:1.3.0-1a3c089
Linux C++ SDK (build): agonessdk-1.3.0-1a3c089-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-1a3c089.zip

A preview of the website (the last 30 builds are retained):

https://1a3c089-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1267/head:pr_1267 && git checkout pr_1267
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-1a3c089

Fix minor issues, but plenty of them. Add new linters. Most changes come from gocritic. Performance was optimised for changing copy of big structures like GameServerSpec into to using a pointer.

agones-bot · 2020-01-13T12:03:27Z

Build Succeeded 👏

Build Id: f65a7360-13c7-42d0-bae8-6f45626e82c3

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.3.0-816fc1d
image: gcr.io/agones-images/agones-sdk:1.3.0-816fc1d
image: gcr.io/agones-images/agones-ping:1.3.0-816fc1d
Linux C++ SDK (build): agonessdk-1.3.0-816fc1d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.3.0-816fc1d.zip

A preview of the website (the last 30 builds are retained):

https://816fc1d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1267/head:pr_1267 && git checkout pr_1267
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-816fc1d

google-oss-robot · 2020-01-13T16:49:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Fix minor issues, but plenty of them. Add new linters. Most changes come from gocritic. Performance was optimised for changing copy of big structures like GameServerSpec into to using a pointer.

google-oss-robot added the do-not-merge/work-in-progress label Jan 10, 2020

google-oss-robot requested review from cyriltovena and EricFortin January 10, 2020 12:46

google-oss-robot added the size/L label Jan 10, 2020

aLekSer force-pushed the update-golangci-lint branch from 97af942 to 6f37730 Compare January 10, 2020 13:22

aLekSer marked this pull request as ready for review January 10, 2020 14:09

google-oss-robot removed the do-not-merge/work-in-progress label Jan 10, 2020

roberthbailey reviewed Jan 10, 2020

View reviewed changes

roberthbailey self-assigned this Jan 10, 2020

roberthbailey removed request for cyriltovena and EricFortin January 10, 2020 18:47

aLekSer force-pushed the update-golangci-lint branch 2 times, most recently from 8682e06 to 644935d Compare January 11, 2020 09:55

aLekSer force-pushed the update-golangci-lint branch 2 times, most recently from f1bc763 to cb01d82 Compare January 11, 2020 10:19

aLekSer force-pushed the update-golangci-lint branch from cb01d82 to f65c6e8 Compare January 11, 2020 12:26

roberthbailey reviewed Jan 13, 2020

View reviewed changes

cmd/allocator/main.go Outdated Show resolved Hide resolved

test/e2e/allocator_test.go Outdated Show resolved Hide resolved

test/e2e/allocator_test.go Outdated Show resolved Hide resolved

aLekSer force-pushed the update-golangci-lint branch from f65c6e8 to 1a3c089 Compare January 13, 2020 08:28

Update golangci-lint, add more linters

816fc1d

Fix minor issues, but plenty of them. Add new linters. Most changes come from gocritic. Performance was optimised for changing copy of big structures like GameServerSpec into to using a pointer.

aLekSer force-pushed the update-golangci-lint branch from 1a3c089 to 816fc1d Compare January 13, 2020 11:48

roberthbailey approved these changes Jan 13, 2020

View reviewed changes

google-oss-robot added the lgtm label Jan 13, 2020

google-oss-robot added the approved label Jan 13, 2020

roberthbailey merged commit c1ba572 into googleforgames:master Jan 13, 2020

markmandel added this to the 1.3.0 milestone Jan 14, 2020

markmandel added area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc labels Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update golangci-lint, add more linters #1267

Update golangci-lint, add more linters #1267

aLekSer commented Jan 10, 2020 •

edited

Loading

agones-bot commented Jan 10, 2020

agones-bot commented Jan 10, 2020

roberthbailey left a comment

roberthbailey Jan 10, 2020

markmandel Jan 11, 2020

aLekSer Jan 11, 2020 •

edited

Loading

roberthbailey Jan 10, 2020

aLekSer Jan 11, 2020 •

edited

Loading

aLekSer Jan 11, 2020

roberthbailey Jan 13, 2020

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

aLekSer commented Jan 11, 2020 •

edited

Loading

agones-bot commented Jan 11, 2020

agones-bot commented Jan 13, 2020

agones-bot commented Jan 13, 2020

google-oss-robot commented Jan 13, 2020

Update golangci-lint, add more linters #1267

Update golangci-lint, add more linters #1267

Conversation

aLekSer commented Jan 10, 2020 • edited Loading

agones-bot commented Jan 10, 2020

agones-bot commented Jan 10, 2020

roberthbailey left a comment

Choose a reason for hiding this comment

roberthbailey Jan 10, 2020

Choose a reason for hiding this comment

markmandel Jan 11, 2020

Choose a reason for hiding this comment

aLekSer Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

roberthbailey Jan 10, 2020

Choose a reason for hiding this comment

aLekSer Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

aLekSer Jan 11, 2020

Choose a reason for hiding this comment

roberthbailey Jan 13, 2020

Choose a reason for hiding this comment

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

agones-bot commented Jan 11, 2020

aLekSer commented Jan 11, 2020 • edited Loading

agones-bot commented Jan 11, 2020

agones-bot commented Jan 13, 2020

agones-bot commented Jan 13, 2020

google-oss-robot commented Jan 13, 2020

aLekSer commented Jan 10, 2020 •

edited

Loading

aLekSer Jan 11, 2020 •

edited

Loading

aLekSer Jan 11, 2020 •

edited

Loading

aLekSer commented Jan 11, 2020 •

edited

Loading